Add top level permissions to all Github Actions workflows
#6660
Conversation
There was a problem hiding this comment.
Reviewed 26 of 28 files at r1, all commit messages.
Reviewable status: 26 of 28 files reviewed, 1 unresolved discussion (waiting on @faern)
.github/workflows/ios-end-to-end-tests-merge-to-main.yml line 14 at r1 (raw file):
- ios/** permissions:
I'm not sure it's a good idea to move these permissions outside the job, since that would make any (potential future) job in this workflow gain those permissions. I believe it's better to put permissions: {} on the top level and then let each job specific exactly what they need.
There was a problem hiding this comment.
Reviewable status: 26 of 28 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)
.github/workflows/ios-end-to-end-tests-merge-to-main.yml line 14 at r1 (raw file):
Previously, albin-mullvad wrote…
I'm not sure it's a good idea to move these permissions outside the job, since that would make any (potential future) job in this workflow gain those permissions. I believe it's better to put
permissions: {}on the top level and then let each job specific exactly what they need.
In general I agree. I was just being pragmatic since this workflow has a single job. The same pattern exists in two other files in this PR. For example cargo-audit is very unlikely to ever get more jobs. And if it does, creating permissions is pretty core to a CVE checking job. So do you think the same logic holds there?
If so, is there ever a reason to specify non-empty top level permissions? I don't see the point in always specifying empty top level permissions and then re-define them with more permissions for workflows that very clearly do one thing and have one job.
There was a problem hiding this comment.
Reviewed 2 of 28 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)
.github/workflows/ios-end-to-end-tests-merge-to-main.yml line 14 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
In general I agree. I was just being pragmatic since this workflow has a single job. The same pattern exists in two other files in this PR. For example
cargo-auditis very unlikely to ever get more jobs. And if it does, creating permissions is pretty core to a CVE checking job. So do you think the same logic holds there?If so, is there ever a reason to specify non-empty top level permissions? I don't see the point in always specifying empty top level permissions and then re-define them with more permissions for workflows that very clearly do one thing and have one job.
Alright, good point, as long as we are aware of the impact of this PR. I'm fine with keeping this change as-is, but I think we need to be careful doing the same in bigger workflows.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad and @faern)
.github/workflows/ios-end-to-end-tests-merge-to-main.yml line 14 at r1 (raw file):
Previously, albin-mullvad wrote…
Alright, good point, as long as we are aware of the impact of this PR. I'm fine with keeping this change as-is, but I think we need to be careful doing the same in bigger workflows.
I had a small chat with @albin-mullvad on this, I also agree with his conclusion.
AFAICT, as long as the default permissions for the GITHUB_TOKEN are set to restrictive, most jobs should have no permissions. (Please take this with a grain of salt, this is my understanding of the documentation, but I'm not 100% sure whether I got it right)
As long as we don't define any new jobs in this yml file, the change should have no impact.
With that being said, I don't have any strong opinions on the changes in this PR, so I'm fine with them.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad and @buggmagnet)
.github/workflows/ios-end-to-end-tests-merge-to-main.yml line 14 at r1 (raw file):
Previously, buggmagnet wrote…
I had a small chat with @albin-mullvad on this, I also agree with his conclusion.
AFAICT, as long as the default permissions for theGITHUB_TOKENare set to restrictive, most jobs should have no permissions. (Please take this with a grain of salt, this is my understanding of the documentation, but I'm not 100% sure whether I got it right)As long as we don't define any new jobs in this
ymlfile, the change should have no impact.With that being said, I don't have any strong opinions on the changes in this PR, so I'm fine with them.
100% agree that we should not just apply a bunch of global permissions to larger/more complex workflows. For workflows with more than one job, where only one job need permissions, we should only give that single job the needed permissions if practical.
faern
force-pushed
the
define-github-actions-workflow-privileges-according-to-least-tec-5
branch
2 times, most recently
from
b1c6ee9
to
f45b3b3
Compare
The default permission on the repository is already set to read only. So in practice this makes no difference. But this makes that more explicit, and less relying on the repository being correctly configured. This also makes security scanning tools such as OpenSSF scorecard happier about the overall security of our repository.
faern
force-pushed
the
define-github-actions-workflow-privileges-according-to-least-tec-5
branch
from
f45b3b3
to
5c93059
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewed 27 of 28 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:
faern
deleted the
define-github-actions-workflow-privileges-according-to-least-tec-5
branch
|
🚨 End to end tests failed. Please check the failed workflow run. |
The default permission on the repository is already set to read only. So in practice this makes no difference. But this makes that more explicit, and less relying on the repository being correctly configured.
This also makes security scanning tools such as OpenSSF scorecard happier about the overall security of our repository.
I think we should start always defining
permissionson our actions. However, we currently don't have anything that enforces it, and given how little actual security it gives us right now it's definitely not worth automating. I just wanted to do this little manual sweep to "improve" the current state.This change is